Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Error reports include line and column number, if available #141

Merged
merged 3 commits into from
Jan 27, 2022
Merged

Error reports include line and column number, if available #141

merged 3 commits into from
Jan 27, 2022

Conversation

mr-c
Copy link
Member

@mr-c mr-c commented Jan 27, 2022

Fixes #129

@codecov
Copy link

codecov bot commented Jan 27, 2022 •
edited
Loading

Codecov Report

Merging #141 (786d2a4) into main (12123b9) will increase coverage by 0.41%.
The diff coverage is 82.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #141      +/-   ##
==========================================
+ Coverage   96.41%   96.82%   +0.41%     
==========================================
  Files           2        4       +2     
  Lines         446      504      +58     
==========================================
+ Hits          430      488      +58     
  Misses         16       16
Impacted Files Coverage Δ
wdl2cwl/main.py 96.17% <58.97%> (+0.06%) ⬆️
wdl2cwl/errors.py 100.00% <100.00%> (ø)
wdl2cwl/tests/test_cwl.py 100.00% <100.00%> (ø)
wdl2cwl/tests/test_wdlsourceline.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12123b9...786d2a4. Read the comment docs.

kinow
kinow approved these changes Jan 27, 2022
View reviewed changes
Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! And I never used that approach to wrap the errors. Will take a better look later to learn/remember it for a future project. Great idea 👍

Approving pending only CI to pass.

@kinow kinow enabled auto-merge January 27, 2022 11:42
@kinow kinow disabled auto-merge January 27, 2022 11:43
@kinow
Copy link
Member

kinow commented Jan 27, 2022

(disabled auto-merge because didn't know if you were going to merge this one first, or merge Dennis' PR first 👍, now 🛌 )

@mr-c mr-c force-pushed the sourceline branch 2 times, most recently from 74dcc47 to ea8fc21 Compare January 27, 2022 11:53
@mr-c mr-c requested a review from ntachukwu January 27, 2022 12:03
@ntachukwu
Copy link
Contributor

This looks cool. So essentially anywhere I am raising an error, I should use this?

ntachukwu
ntachukwu approved these changes Jan 27, 2022
View reviewed changes
Copy link
Contributor

@ntachukwu ntachukwu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will love to make use of it.

@mr-c
Copy link
Member Author

mr-c commented Jan 27, 2022

This looks cool. So essentially anywhere I am raising an error, I should use this?

Yes, and also where some other code might generate an exception related to a specific miniWDL object:

https://github.com/common-workflow-lab/wdl-cwl-translator/pull/141/files#diff-b5a65c2befbbd0b6453805272729b622f41b2be683f8258432e08f4370332265R162

https://github.com/common-workflow-lab/wdl-cwl-translator/pull/141/files#diff-b5a65c2befbbd0b6453805272729b622f41b2be683f8258432e08f4370332265R175

https://github.com/common-workflow-lab/wdl-cwl-translator/pull/141/files#diff-b5a65c2befbbd0b6453805272729b622f41b2be683f8258432e08f4370332265R358

https://github.com/common-workflow-lab/wdl-cwl-translator/pull/141/files#diff-b5a65c2befbbd0b6453805272729b622f41b2be683f8258432e08f4370332265R392

@mr-c mr-c merged commit 6776bbf into main Jan 27, 2022
@mr-c mr-c deleted the sourceline branch January 27, 2022 13:47
@kinow
Copy link
Member

kinow commented Jan 27, 2022 •
edited
Loading

Loved it! Tested the workflow from #77

wdl2cwl.main.ConversionException: wdl2cwl/tests/wdl_files/tasks/star.wdl:67:33: Function name 'select_all' not yet handled.

Opened workflow in IDE, exact line & column reported in the error. Thanks heaps!!

This was referenced Jan 27, 2022
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntachukwu ntachukwu ntachukwu approved these changes

@kinow kinow kinow approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make use of the "pos" attribute on MiniWDL objects for better error messages
3 participants
@mr-c @kinow @ntachukwu